New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes escaping port number in absolute urls #1028
Conversation
tdeekens
commented
Dec 13, 2021
- Closes Cannot match absolute URLs with port number #1027
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4563b6c:
|
* Escape the port so that "path-to-regexp" can match | ||
* absolute URLs including port numbers. | ||
*/ | ||
.replace(/((?::)(?:[0-9]+))/g, '\\$1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively the following should also work /(:[0-9]+)/
. @kettanaito you can pick :)
@@ -104,5 +104,8 @@ describe('coercePath', () => { | |||
expect(coercePath('https://example.com:1234/:userId')).toEqual( | |||
'https\\://example.com\\:1234/:userId', | |||
) | |||
expect(coercePath('https://example.com:1234/:5678')).toEqual( | |||
'https\\://example.com\\:1234/:5678', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how to deal with this case. Suggestions welcome.
An alternative approach, not using Regular Expressions, could be to use |
1fef75e
to
7a22520
Compare
7a22520
to
4563b6c
Compare
Thanks for opening this, @tdeekens! I've adjusted the regular expression to match the port number pattern while allowing leading numbers in the path parameter names (as well as parameter names represented by numbers only). I've also changed the order in which the port is escaped, so it happens before escaping the protocol. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for helping fix this 👏